-
Notifications
You must be signed in to change notification settings - Fork 42
Make Github messages more readable #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Github messages more readable #34
Conversation
This is wonderful, thank you! ✨ Have you run this with your production hubot perchance? We don't have any tests for this (heh) so if you could try it and let me know that it's working for you in all the changed cases, that would be wonderful. 😄 Also, is this syntax a Mattermost-specific syntax? If so, do you know if there's a way to make this platform-agnostic? Slack, Mattermost, and Campfire all have their own link formats. |
I'm so glad you like it! 😄 Yes, I made these changes in our production hubot first, and we have been using it like this for about a week. So far we didn't have any problems. About the syntax, that's a good point. I checked Slack's documentation and apparently they use the same link format as Mattermost: But I can't find any documentation on how Campfire or HipChat handle links (let alone all the other platforms), so I'm guessing they probably don't have this option. I think one possibility would be to have a separate class that would receive the data object and return the formatted message according to the current adapter. What do you think? |
I think that sounds great! I believe |
Done! I'm not sure I did it in the best possible way, because I'm just starting to get into this node/coffee/hubot world (I'm originally a Rails developer), so let me know if there's a better way to accomplish this. :) But in any case, it works! The links are still showing nicely in Mattermost and I tested it the other way around too (displaying only the URL when the adapter is Mattermost), and it also works. Now it's just a matter of adding "whens" to the switch and it should work for any adapter! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much! Just one nit about escaping the hash symbol and a comment about the order of parameters. Code looks solid though! 👍
issue = data.issue | ||
comment = data.comment | ||
repo = data.repository | ||
repo_link = formatUrl adapter, repo.html_url, repo.name | ||
comment_link = formatUrl adapter, comment.html_url, "#{issue_pull} \##{issue.number}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you have to escape the #
symbol in CoffeeScript. The code you have here, per coffeescript.org, produces:
issue_pull + " \#" + issue.number;
pull_num = data.number | ||
pull_req = data.pull_request | ||
base = data.base | ||
repo = data.repository | ||
repo_link = formatUrl adapter, repo.html_url, repo.name | ||
pull_request_link = formatUrl adapter, pull_req.html_url, "\##{data.number} \"#{pull_req.title}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to escape the first #
here either.
if eventActions[eventType]? | ||
eventActions[eventType](data, cb) | ||
eventActions[eventType](data, cb, adapter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this though it's usually customary (from what I remember) for the callback (cb
here) to be the last parameter. That is kind of annoying to go back and change so if you don't want to do it, no sweat. 😄
Done! I fixed the order of the parameters too, it was no trouble at all 😄 |
This is GREAT! ✨ 🎉 💖 Thank you so much for your contribution and for sticking with me through my reviews. I'll merge and release this now! |
Looks like I can't release to NPM. For now, you can add this to your package.json: "hubot-github-repo-event-notifier": "https://github.com/hubot-scripts/hubot-github-repo-event-notifier.git#v1.7.0" I think that should work. :) |
Awesome!!!!! 🎉 🎉 🎉 Thank you too, for taking the time to review my code! I had a lot of fun with this. I'm getting addicted to Hubot, so I hope to make more contributions soon! |
We've been using this script a lot with Mattermost at the company I work for, and we love it. But since we have many repositories and a lot of daily activity, our code channel was getting a little messy, so I made some changes to the messages to make them more readable. I hope other people might like it as well.